Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Python] Function Keyword Argument and Function Call through Parameter PR #644

Merged
merged 20 commits into from
Nov 18, 2023

Conversation

titomeister
Copy link
Contributor

@titomeister titomeister commented Nov 14, 2023

GroMEt generation

  • Added support for handling the GroMEt primitive "_call"
  • Fixed an issue with the unpacking of a tuple from an attribute item
  • Fixed an issue with packing of a tuple in function calls.
  • Added some additional code to make the handling of Attributes more robust
  • Fixed an issue with functions being used as keyword arguments
  • Fixed an issue with port indices being too large in some wires.

Python to CAST

  • Added a table to maintain a function's arguments. This is used to determine if an argument is being called as a function.
  • Added support for generating a primitive "_call" Function Call node that's used whenever a function that's passed as a function parameter is being called.

Other

  • Added "_call" primitive to the execution engine's set of primitives in order to properly support GroMEt generation for it.

Testing

  • Added a test file that checks that the generation and use of the "_call" primitive is correct and consistent.

Resolves #592

@titomeister titomeister added this to the [DARPA] Milestone 11 milestone Nov 14, 2023
@titomeister titomeister changed the title [Python] Function Keyword Argument and Function Call through Parameter [Python] Function Keyword Argument and Function Call through Parameter PR Nov 14, 2023
@github-actions github-actions bot removed the Code2FN label Nov 14, 2023
@@ -129,3 +129,16 @@ class Range:

def exec(input: int) -> range:
return range(input)


class Call:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We eventually will want to stop using the primitives in skema/gromet/execution_engine/types/ since they are artifacts left over from an outdated version of the execution engine. We have a YAML file which I believe should compile the same information in a more approachable way at skema/gromet/primitive_map.yaml

How large of a change would this require for the Gromet generation?

This is also something we may want to combine with python_builtins.yaml or create a home directory for these type of files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay. I had forgotten about that. I think as long as we can use the same interface all that would be needed would be to update what's going on behind the scenes. That is, I just need to change the interface to pull from the correct primitive map file. I don't expect there to be any reason to explicitly change anything in the Gromet generation code itself.

foo(x=a,y=b,z=1)
"""

def generate_gromet(test_file_string):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a minor thing, but since this function is defined in multiple test files, it may be useful to add it to skema/utils

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, there's a lot of repetitive things in the test cases that I would eventually like to add there. This is another good thing to add though.

@myedibleenso myedibleenso added bug Something isn't working Code2FN tests Issues related to existing or additional tests lang/Python Used for work related to Python (ex. frontend support) labels Nov 15, 2023
@github-actions github-actions bot removed Code2FN tests Issues related to existing or additional tests lang/Python Used for work related to Python (ex. frontend support) labels Nov 15, 2023
@myedibleenso
Copy link
Collaborator

@titomeister , can you please run python skema/program_analysis/model_coverage_report/model_coverage_report.py locally on this branch and report overall coverage stats for our models? I just want to know if the import issue is going to reduce our coverage at all.

@titomeister
Copy link
Contributor Author

titomeister commented Nov 16, 2023

@titomeister , can you please run python skema/program_analysis/model_coverage_report/model_coverage_report.py locally on this branch and report overall coverage stats for our models? I just want to know if the import issue is going to reduce our coverage at all.

I run into this error when I run that script. I'm not sure what the cause is at this time.

/Users/ferra/.venvs/skema/lib/python3.8/site-packages/pydantic/_internal/_config.py:261: UserWarning: Valid config keys have changed in V2: 'schema_extra' has been renamed to 'json_schema_extra' warnings.warn(message, UserWarning) Traceback (most recent call last): File "skema/program_analysis/model_coverage_report/model_coverage_report.py", line 31, in <module> from skema.rest.utils import fn_preprocessor File "/Users/ferra/Desktop/Work_Repos/skema/skema/rest/utils.py", line 4, in <module> from askem_extractions.data_model import AttributeCollection, AttributeType, Mention ImportError: cannot import name 'Mention' from 'askem_extractions.data_model' (/Users/ferra/.venvs/skema/lib/python3.8/site-packages/askem_extractions/data_model/__init__.py)

@vincentraymond-ua
Copy link
Contributor

@titomeister - Could you reinstall Skema and see if that error goes away? I have a feeling that this is something that may have been added since you last installed the project.

@jastier
Copy link
Contributor

jastier commented Nov 16, 2023

This is a bug in my code I believe. I am now putting up a PR that should fix it.

@titomeister
Copy link
Contributor Author

@titomeister - Could you reinstall Skema and see if that error goes away? I have a feeling that this is something that may have been added since you last installed the project.

Thank you @vincentraymond-ua, that resolved that problem! Working on some stuff with the report it generated now.

Copy link
Contributor

@vincentraymond-ua vincentraymond-ua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@titomeister - Looks good! Sorry for taking so long to re-review it.

@titomeister
Copy link
Contributor Author

@titomeister - Looks good! Sorry for taking so long to re-review it.

No worries! Thank you for looking at it.

@myedibleenso myedibleenso merged commit bd15457 into main Nov 18, 2023
8 checks passed
@myedibleenso myedibleenso deleted the ferra/climlab_func_def_issue branch November 18, 2023 00:22
github-actions bot added a commit that referenced this pull request Nov 18, 2023
…r PR (#644)

### GroMEt generation
- Added support for handling the GroMEt primitive "_call"
- Fixed an issue with the unpacking of a tuple from an attribute item
- Fixed an issue with packing of a tuple in function calls.
- Added some additional code to make the handling of Attributes more
robust
- Fixed an issue with functions being used as keyword arguments
- Fixed an issue with port indices being too large in some wires.

### Python to CAST
- Added a table to maintain a function's arguments. This is used to
determine if an argument is being called as a function.
- Added support for generating a primitive "_call" Function Call node
that's used whenever a function that's passed as a function parameter is
being called.

### Other
- Added "_call" primitive to the execution engine's set of primitives in
order to properly support GroMEt generation for it.

### Testing
- Added a test file that checks that the generation and use of the
"_call" primitive is correct and consistent.

Resolves #592

---------

Co-authored-by: Vincent Raymond <[email protected]>
Co-authored-by: Gus Hahn-Powell <[email protected]> bd15457
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[code2fn] SIDARTHE Keyword Argument Bug
4 participants